introduce `MASKING` operator in `triggerExpression::Parser` #39196

missirol · 2022-08-25T16:27:29Z

PR description:

This PR aims to add a new operator to the syntax supported by the triggerExpression::Parser class.

The new operator is binary, and tentatively [0] named MASKING.

Given a generic expression ExprFoo (e.g. L1_A* OR L1_B*) and a generic pattern PattBar (e.g. L1*copy*), ExprFoo MASKING PattBar corresponds to evaluating ExprFoo disabling in it ("masking") all the triggers matching PattBar.

More details on the syntax are given in [1].

A use case for this comes from HLT online operations [2], see for example CMSHLT-2403.

The idea to introduce a new logical operator to solve this issue comes from @fwyzard.

Unit tests in HLT packages are updated accordingly.

Merely technical. No changes expected.

[0] Feel free to suggest a better name (it can be changed, of course); I just found MASKING slightly clearer than EXCEPT or BUT.

[1] Esp. during commissioning, "copies" of some L1T seeds are used online for tests (e.g. having L1_ZeroBias_copy with loose PSs for high-rate tests, leaving L1_ZeroBias and the HLTs seeded by it unaffected), while HLT tipically wants to ignore such "copy" seeds in the collection of "HLTPhysics" data (L1A Physics triggers). For this reason, right now in the online menu the (Ephemeral)HLTPhysics triggers are seeded by a EDFilter containing (for lack of a better syntax) the explicit list of all L1T seeds which are not "copy" seeds (over 300 strings), which is clearly difficult to maintain. Using the equivalent L1_* MASKING L1_ZeroBias_copy would be the better approach.

[2] Behaviour of MASKING operator:

In (PATTERN_A OR/AND PATTERN_B) MASKING PATTERN_C, all matches of PATTERN_C are masked in both PATTERN_A and PATTERN_B.
The argument of the MASKING operation (i.e. B in A MASKING B) can be only a single pattern corresponding to L1T seeds (class: TriggerExpressionL1uGTReader) or HLT paths (class: TriggerExpressionPathReader); the argument cannot be a constant (TRUE or FALSE), a prescale expression (e.g. X / N), nor a unary or a binary operator; if unsupported arguments are used, e.g. ([..] MASKING (THIS AND THAT)), the parsing will fail. Examples of unsupported expressions: EXPR_A MASKING (PATTERN_B AND/OR PATTERN_C), EXPR_A MASKING (NOT PATTERN_B), EXPR_A MASKING TRUE, EXPR_A MASKING (PATH / N).

PR validation:

Improved unit tests pass.

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

TSG might want to backport this new feature down to CMSSW_12_4_X for online operations.

cmsbuild · 2022-08-25T16:35:16Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39196/31807

This PR adds an extra 24KB to repository

cmsbuild · 2022-08-25T16:35:38Z

A new Pull Request was created by @missirol (Marino Missiroli) for master.

It involves the following packages:

HLTrigger/HLTcore (hlt)
HLTrigger/HLTfilters (hlt)

@cmsbuild, @missirol, @Martin-Grunewald can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @silviodonato, @fwyzard this is something you requested to watch as well.
@perrotta, @dpiparo, @qliphy, @rappoccio you are the release manager for this.

cms-bot commands are listed here

fwyzard · 2022-08-29T22:57:42Z

Can multiple MASKING operands be chained ?

For example, what happens with L1_* MASKING L1_Jet* MASKING L1_Muon* ?

fwyzard · 2022-08-29T23:00:07Z

HLTrigger/HLTcore/interface/TriggerExpressionL1uGTReader.h

+    void maskTriggers(L1uGTReader const&);
+
+    bool useTriggersAfterMasking() const { return m_useTriggersAfterMasking; }
+    void useTriggersAfterMasking(bool const foo) { m_useTriggersAfterMasking = foo; }


IIRC the general recommendation is to use

bool useTriggersAfterMasking() const { return m_useTriggersAfterMasking; } void setUseTriggersAfterMasking(bool const foo) { m_useTriggersAfterMasking = foo; }

fwyzard · 2022-08-29T23:02:24Z

HLTrigger/HLTcore/interface/TriggerExpressionOperators.h

+    bool operator()(const Data& data) const override {
+      // force the execution of both arguments, otherwise prescalers won't work properly
+      bool r1 = (*m_arg1)(data);
+      (*m_arg2)(data);


Is this needed, if MASKING does not support prescaled right-hand arguments ?

I would say no, indeed, but I wasn't 100% sure about it because of the comment on the prescalers. Since a prescale operator cannot be on the right side of MASKING in any case, I think it's safe to remove this.

missirol · 2022-08-30T06:16:58Z

Can multiple MASKING operands be chained ?

For example, what happens with L1_* MASKING L1_Jet* MASKING L1_Muon* ?

Yes, this is equivalent to (L1_* MASKING L1_Jet*) MASKING L1_Muon*; there is such an example in the unit tests, e.g. here.

I think the sequence of calls is the following:

the expression (EXPR_A MASKING EXPR_B) is initialised (init), and we have "EXPR_A".mask("EXPR_B");
the expression (EXPR_A MASKING EXPR_B) MASKING EXPR_C is initialised; (EXPR_A MASKING EXPR_B) is of type OperatorMasking which inherits ::mask from OperatorBinary::mask and so
"(EXPR_A MASKING EXPR_B)".mask("EXPR_C") corresponds to "EXPR_A".mask("EXPR_C") (where EXPR_A already includes the masking of EXPR_B) plus "EXPR_B".mask("EXPR_C"); the latter call has no practical effect, thus is a bit unnecessary; it could be avoided by implementing void OperatorMasking::mask as { m_arg1->mask(arg); }, I think.

missirol · 2022-08-30T11:45:10Z

[..] the latter call has no practical effect, thus is a bit unnecessary; it could be avoided by implementing void OperatorMasking::mask as { m_arg1->mask(arg); }, I think.

I tried this, and it works as expected (no changes).

I plan to apply the following changes in the next push, based on the comments by Andrea.

diff --git a/HLTrigger/HLTcore/interface/TriggerExpressionL1uGTReader.h b/HLTrigger/HLTcore/interface/TriggerExpressionL1uGTReader.h
index 038c30fe4a9..1306de60952 100644
--- a/HLTrigger/HLTcore/interface/TriggerExpressionL1uGTReader.h
+++ b/HLTrigger/HLTcore/interface/TriggerExpressionL1uGTReader.h
@@ -32,7 +32,7 @@ namespace triggerExpression {
     void maskTriggers(L1uGTReader const&);
 
     bool useTriggersAfterMasking() const { return m_useTriggersAfterMasking; }
-    void useTriggersAfterMasking(bool const foo) { m_useTriggersAfterMasking = foo; }
+    void setUseTriggersAfterMasking(bool const foo) { m_useTriggersAfterMasking = foo; }
 
   private:
     std::string m_pattern;
diff --git a/HLTrigger/HLTcore/interface/TriggerExpressionOperators.h b/HLTrigger/HLTcore/interface/TriggerExpressionOperators.h
index 1a0c73bf362..2bdeb15a828 100644
--- a/HLTrigger/HLTcore/interface/TriggerExpressionOperators.h
+++ b/HLTrigger/HLTcore/interface/TriggerExpressionOperators.h
@@ -136,12 +136,7 @@ namespace triggerExpression {
   public:
     OperatorMasking(Evaluator* arg1, Evaluator* arg2) : BinaryOperator(arg1, arg2) {}
 
-    bool operator()(const Data& data) const override {
-      // force the execution of both arguments, otherwise prescalers won't work properly
-      bool r1 = (*m_arg1)(data);
-      (*m_arg2)(data);
-      return r1;
-    }
+    bool operator()(const Data& data) const override { return (*m_arg1)(data); }
 
     void init(const Data& data) override {
       m_arg1->init(data);
@@ -149,6 +144,10 @@ namespace triggerExpression {
       m_arg1->mask(m_arg2.get());
     }
 
+    // apply mask(s) to the first Evaluator
+    // (the 2nd Evaluator is not used anyway in the decision of OperatorMasking)
+    void mask(Evaluator* arg) override { m_arg1->mask(arg); }
+
     void dump(std::ostream& out) const override {
       out << '(';
       m_arg1->dump(out);
diff --git a/HLTrigger/HLTcore/interface/TriggerExpressionPathReader.h b/HLTrigger/HLTcore/interface/TriggerExpressionPathReader.h
index 6c3e24039c7..51ec0a3aea9 100644
--- a/HLTrigger/HLTcore/interface/TriggerExpressionPathReader.h
+++ b/HLTrigger/HLTcore/interface/TriggerExpressionPathReader.h
@@ -34,7 +34,7 @@ namespace triggerExpression {
     void maskTriggers(PathReader const&);
 
     bool useTriggersAfterMasking() const { return m_useTriggersAfterMasking; }
-    void useTriggersAfterMasking(bool const foo) { m_useTriggersAfterMasking = foo; }
+    void setUseTriggersAfterMasking(bool const foo) { m_useTriggersAfterMasking = foo; }
 
   private:
     std::string m_pattern;

cmsbuild · 2022-08-30T16:32:45Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39196/31893

This PR adds an extra 12KB to repository

cmsbuild · 2022-08-30T16:33:14Z

Pull request #39196 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

cmsbuild · 2022-08-30T16:42:17Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39196/31894

This PR adds an extra 16KB to repository

cmsbuild · 2022-09-07T08:38:33Z

+code-checks

Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39196/32029

This PR adds an extra 24KB to repository

cmsbuild · 2022-09-07T08:38:53Z

Pull request #39196 was updated. @cmsbuild, @missirol, @Martin-Grunewald can you please check and sign again.

missirol · 2022-09-07T08:39:21Z

The last push simply rebases the PR on a recent IB.

Unless there are comments, I plan to move forward with its integration in 12_6_X, and backport it to 12_5_X and 12_4_X.

missirol · 2022-09-07T08:40:14Z

please test

cmsbuild · 2022-09-07T13:15:35Z

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-afde59/27395/summary.html
COMMIT: aa32f59
CMSSW: CMSSW_12_6_X_2022-09-06-2300/el8_amd64_gcc10
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week1/cms-sw/cmssw/39196/27395/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

No significant changes to the logs found
Reco comparison results: 8 differences found in the comparisons
DQMHistoTests: Total files compared: 51
DQMHistoTests: Total histograms compared: 3618210
DQMHistoTests: Total failures: 14
DQMHistoTests: Total nulls: 0
DQMHistoTests: Total successes: 3618174
DQMHistoTests: Total skipped: 22
DQMHistoTests: Total Missing objects: 0
DQMHistoSizes: Histogram memory added: 0.0 KiB( 50 files compared)
Checked 212 log files, 49 edm output root files, 51 DQM output files
TriggerResults: no differences found

missirol · 2022-09-08T13:16:00Z

+hlt

this PR adds a new logical operator to the triggerExpression::Parser class
the new operator allows to "mask" (or, ignore) triggers inside a given logical expression; for example, L1_* MASKING L1_Zero*copy corresponds to the OR of "all L1 seeds except for the ones matching the name pattern L1_Zero*copy"
this PR does not modify the outputs of any existing wf, and the new feature it introduces is currently unused
since this new feature might be used by TSG during online operations in 2022, this PR will be backported to 12_5_X and 12_4_X
unit tests are updated accordingly

cmsbuild · 2022-09-08T13:16:24Z

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2)

perrotta · 2022-09-08T13:48:46Z

+1

cmsbuild added this to the CMSSW_12_5_X milestone Aug 25, 2022

cmsbuild added code-checks-pending hlt-pending orp-pending pending-signatures tests-pending labels Aug 25, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 25, 2022

smuzaffar modified the milestones: CMSSW_12_5_X, CMSSW_12_6_X Aug 28, 2022

fwyzard reviewed Aug 29, 2022

View reviewed changes

missirol force-pushed the devel_hltParserMaskOperator_3 branch from e8988d9 to 0bd9d8e Compare August 30, 2022 16:25

cmsbuild added code-checks-pending and removed code-checks-approved labels Aug 30, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 30, 2022

missirol force-pushed the devel_hltParserMaskOperator_3 branch from 0bd9d8e to ba2e4b3 Compare August 30, 2022 16:35

cmsbuild added code-checks-pending and removed code-checks-approved labels Aug 30, 2022

cmsbuild added code-checks-approved and removed code-checks-pending labels Aug 30, 2022

missirol force-pushed the devel_hltParserMaskOperator_3 branch from 626c61c to 5cd0ad7 Compare September 7, 2022 08:31

cmsbuild added code-checks-pending and removed code-checks-approved labels Sep 7, 2022

introduce MASKING operator in triggerExpression::Parser

aa32f59

missirol force-pushed the devel_hltParserMaskOperator_3 branch from 5cd0ad7 to aa32f59 Compare September 7, 2022 08:38

cmsbuild added code-checks-approved and removed code-checks-pending labels Sep 7, 2022

cmsbuild added tests-started and removed tests-pending labels Sep 7, 2022

cmsbuild added tests-approved and removed tests-started labels Sep 7, 2022

cmsbuild added fully-signed hlt-approved and removed hlt-pending pending-signatures labels Sep 8, 2022

cmsbuild added orp-approved and removed orp-pending labels Sep 8, 2022

cmsbuild merged commit 435424d into cms-sw:master Sep 8, 2022

This was referenced Sep 8, 2022

introduce MASKING operator in triggerExpression::Parser [12_5_X] #39348

Merged

introduce MASKING operator in triggerExpression::Parser [12_4_X] #39349

Merged

missirol deleted the devel_hltParserMaskOperator_3 branch September 8, 2022 21:11

cmsbuild mentioned this pull request Sep 9, 2022

[BuildRules] Updated unit test startup command cms-sw/cmsdist#8075

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce `MASKING` operator in `triggerExpression::Parser` #39196

introduce `MASKING` operator in `triggerExpression::Parser` #39196

missirol commented Aug 25, 2022 •

edited

Loading

cmsbuild commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

fwyzard commented Aug 29, 2022

fwyzard Aug 29, 2022

fwyzard Aug 29, 2022 •

edited

Loading

missirol Aug 30, 2022

missirol commented Aug 30, 2022

missirol commented Aug 30, 2022

cmsbuild commented Aug 30, 2022

cmsbuild commented Aug 30, 2022

cmsbuild commented Aug 30, 2022

cmsbuild commented Sep 7, 2022

cmsbuild commented Sep 7, 2022

missirol commented Sep 7, 2022

missirol commented Sep 7, 2022

cmsbuild commented Sep 7, 2022

missirol commented Sep 8, 2022

cmsbuild commented Sep 8, 2022

perrotta commented Sep 8, 2022

introduce MASKING operator in triggerExpression::Parser #39196

introduce MASKING operator in triggerExpression::Parser #39196

Conversation

missirol commented Aug 25, 2022 • edited Loading

PR description:

PR validation:

If this PR is a backport, please specify the original PR and why you need to backport that PR. If this PR will be backported, please specify to which release cycle the backport is meant for:

cmsbuild commented Aug 25, 2022

cmsbuild commented Aug 25, 2022

fwyzard commented Aug 29, 2022

fwyzard Aug 29, 2022

Choose a reason for hiding this comment

fwyzard Aug 29, 2022 • edited Loading

Choose a reason for hiding this comment

missirol Aug 30, 2022

Choose a reason for hiding this comment

missirol commented Aug 30, 2022

missirol commented Aug 30, 2022

cmsbuild commented Aug 30, 2022

cmsbuild commented Aug 30, 2022

cmsbuild commented Aug 30, 2022

cmsbuild commented Sep 7, 2022

cmsbuild commented Sep 7, 2022

missirol commented Sep 7, 2022

missirol commented Sep 7, 2022

cmsbuild commented Sep 7, 2022

Comparison Summary

missirol commented Sep 8, 2022

cmsbuild commented Sep 8, 2022

perrotta commented Sep 8, 2022

introduce `MASKING` operator in `triggerExpression::Parser` #39196

introduce `MASKING` operator in `triggerExpression::Parser` #39196

missirol commented Aug 25, 2022 •

edited

Loading

fwyzard Aug 29, 2022 •

edited

Loading